Cartesian subplot updates using data joins #946

etpinard · 2016-09-15T21:33:28Z

resolves #634

In trying to make the cartesian plot routine a little more reusable for range slider 2.0, I ran into these

// in plot_api.js
var subplots = Plotly.Axes.getSubplots(gd).join(''),
      oldSubplots = Object.keys(gd._fullLayout._plots || {}).join(''),
      hasSameSubplots = (oldSubplots === subplots);

if(!hasSameSubplots) makePlotFramework(gd);

which basically destroys and redraw all axes every time the subplot layout changes. We could do better.

In this PR, I attempt to move the subplot creation / updates out of the makePlotFramework and into the Cartesian module using a d3 data join. In doing so, fullLayout._plots logic is cleaned up considerably (resolving issue #634).

cc @rreusser @alexcjohnson

- which handles fullLayout._plots creation and reference updates - rm block in Plots.transition that used to handle that

- we must now call drawAxes AFTER drawData (i.e. after updateSubplots) as drawAxes looks for some <g> that updateSubplots creates - move layoutStyles / Fx.init out of makePlotFramework - make makePlotFramework return 'FRAMEWORK' so that restyle polar <-> cartesian still works

etpinard · 2016-09-15T21:39:04Z

This PR is currently in progress. Some image test mocks and a few relayout tests are failing.

Most of these issues are - I believe - caused by Axes.doTicks which computes and mutates a few axis properties as well as drawing ticks - which turned out to be order dependent.

To note also, subroutine layoutStyles also mutates an axis property (this time _linepositions).

- so that these ref on fullLayout exist even for non-cartesian trace types.

- after several failed attempts at getting cartesian subplot creation/update/removal into the Cartesian.plot (like the other base plot module), I decide to go with a less ambitious refactoring where a drawFramework update step is added to the main Plotly.plot code path - Main reason: several layout components require the cartesian framework to be present in order to work properly.

- this suite used to rely on className 'bg' to grab the legend background element, but now className 'bg' is also used in <g subplot>.

- doCalcdata is now called before drawFramwework, so the ordering for which _ keys are added to the gd differs slightly.

etpinard · 2016-09-16T22:25:23Z

src/plots/cartesian/index.js

+    subplotLayers.enter().append('g')
+        .classed('subplot', true);
+
+    subplotLayers.order();


See http://codepen.io/etpinard/pen/mAEBVz for example of this pattern

etpinard · 2016-09-16T22:31:27Z

In commit 937c54b I settled for a less ambitious refactoring attempt in this PR.

It turns out that (parts of) the cartesian framework are required in order to make (some) layout components draw properly. Meaning that we must draw the cartesian framework before getting to the main basePlotModules loop in the drawData sub-task - unlike other subplot types.

So, I decided to a drawFramework method on the Cartesian module object.

In a future PR(s), we should spent some time refactoring axes.js where several drawing methods mutate the axis objects, making it really hard to know what step should come first. In other words, we should try to refactor axes.js into calc steps and draw steps.

etpinard · 2016-09-16T22:38:58Z

test/jasmine/tests/plots_test.js

@@ -54,7 +54,7 @@ describe('Test Plots', function() {
            expect(gd._fullData[1].z).toBe(newData[1].z);
            expect(gd._fullData[1]._empties).toBe(oldFullData[1]._empties);
            expect(gd._fullLayout.scene._scene).toBe(oldFullLayout.scene._scene);
-            expect(gd._fullLayout._plots).toBe(oldFullLayout._plots);
+            expect(gd._fullLayout._plots.plot).toBe(oldFullLayout._plots.plot);


_plots is no longer relinked in Plots.supplyDefaults as

newFullLayout._plots = oldFullLayout._plots

but as

var oldSubplots = oldFullLayout._plots || {}, newSubplots = newFullLayout._plots = {}; var ids = Plotly.Axes.getSubplots(mockGd); for(var i = 0; i < ids.length; i++) { var id = ids[i], oldSubplot = oldSubplots[id], plotinfo; if(oldSubplot) { plotinfo = newSubplots[id] = oldSubplot; } else { plotinfo = newSubplots[id] = {}; plotinfo.id = id; } plotinfo.x = getAxisFunc(id, 'x'); plotinfo.y = getAxisFunc(id, 'y'); plotinfo.xaxis = plotinfo.x(); plotinfo.yaxis = plotinfo.y(); }

cc @rreusser

So it looks like it reconstructs a new _plots object with the same contents vs. just transferring it? Is that what makes the join work correctly?

it looks like it reconstructs a new _plots object with the same contents vs. just transferring it?

Correct. Thanks for the headsup. Maybe we could do better.

Is that what makes the join work correctly?

I don't think reconstructing _plots is necessary to make the joins work correctly.

etpinard · 2016-09-16T22:40:22Z

test/jasmine/tests/legend_scroll_test.js


            Plotly.addTraces(gd, { x: [1, 2, 3], y: [4, 3, 2], name: 'Test2' }).then(function() {
-                legend = document.getElementsByClassName('legend')[0];
-                bg = document.getElementsByClassName('bg')[0];


The subplot bg layer now also has class bg, which made this query grab the wrong node.

etpinard · 2016-09-16T22:42:05Z

src/plot_api/plot_api.js

-    //  the new layout gets ignored (as it should)
-    //  but if there's no data there yet, it's just a placeholder...
-    //  then it should destroy and remake the plot
-    if(hasData) {


@alexcjohnson can you review these few lines?

Can you think of any other (edge) cases that I could have missed?

etpinard · 2016-09-16T22:43:17Z

src/plot_api/plot_api.js

+            [] :
+            [marginPushers, subroutines.layoutStyles];
+
+        return Lib.syncOrAsync(seq, gd);


no need to call Fx.init here too as it is called in drawFramework on every Plotly.plot.

Previously, we need a change of subplot in order to trigger Fx.init.

etpinard · 2016-09-16T22:43:50Z

src/plots/cartesian/index.js

+}
+
+
+function joinLayer(parent, nodeType, className) {


Good enough for Lib function ???

Yes, def. We use this pattern all over the place, and it's invalid in d3 v4 without some minor modifications, so would be good to extract.

I'll defer that to a future PR.

etpinard · 2016-09-16T22:45:10Z

src/plots/plots.js

@@ -559,6 +562,41 @@ function relinkPrivateKeys(toContainer, fromContainer) {
    }
 }

+plots.linkSubplots = function(newFullData, newFullLayout, oldFullData, oldFullLayout) {


This routine here should make fullLayout._plots[' '].x() and fullLayout._plots[' '].y() method obsolete.

cc @rreusser

done in 2b05978

etpinard · 2016-09-16T22:46:26Z

src/plots/plots.js

+        }
+
+        plotinfo.x = getAxisFunc(id, 'x');
+        plotinfo.y = getAxisFunc(id, 'y');


Here they are for backward-compatibility.

It might be worth the time to remove all x() and y() and replace them with _plots[''].xaxis and _plots[''].yaxis respectively.

I meant here. done in 2b05978

rreusser

I don't see anything blocking here. I don't understand every bit of it since I haven't gotten too deep into the plot framework, but I like the change, so if it passes the tests, then I say 💃🏻

rreusser · 2016-10-05T18:57:50Z

src/components/errorbars/plot.js

-    var xa = plotinfo.x(),
-        ya = plotinfo.y();
+
+    var xa = plotinfo.xaxis,


Is plotinfo.x() removed here or in general? I had some subtle problems with one or the other being up to date as axis references change.

rreusser · 2016-10-05T19:01:19Z

src/plot_api/plot_api.js

-    }
-    else if(!hasSameSubplots) {
+    // polar need a different framework
+    if(gd.framework !== makePlotFramework) {


The naming is a bit confusing to me since framework = noun while makePlotFramework = verb, but this is probably nothing new and not a big deal.

I agree this is very strange. But, like pointed out by the comment above, the only reason this line is here is because of polar.

As polar is so broken at the moment, I prefer not changing things too much until we fully refactor it.

rreusser · 2016-10-05T19:06:17Z

src/plot_api/plot_api.js

-            .style('fill', 'none')
-            .classed('crisp', true);
-    });
+    return 'FRAMEWORK';


What does this return code do? It seems 10% strange, so maybe a comment would be good if it's actually needed/expected.

Oh good catch.

There's no need for this line anymore.

rreusser · 2016-10-05T19:08:23Z

src/plots/cartesian/axes.js

@@ -1369,8 +1373,8 @@ axes.doTicks = function(gd, axid, skipTitle) {
        if(axid === 'redraw') {
            fullLayout._paper.selectAll('g.subplot').each(function(subplot) {
                var plotinfo = fullLayout._plots[subplot],
-                    xa = plotinfo.x(),
-                    ya = plotinfo.y();
+                    xa = plotinfo.xaxis,


Big fan of this change! 🍻

rreusser · 2016-10-05T19:11:15Z

src/plots/cartesian/index.js

+}
+
+
+function joinLayer(parent, nodeType, className) {


Yes, def. We use this pattern all over the place, and it's invalid in d3 v4 without some minor modifications, so would be good to extract.

rreusser · 2016-10-05T19:15:39Z

test/jasmine/tests/plots_test.js

@@ -54,7 +54,7 @@ describe('Test Plots', function() {
            expect(gd._fullData[1].z).toBe(newData[1].z);
            expect(gd._fullData[1]._empties).toBe(oldFullData[1]._empties);
            expect(gd._fullLayout.scene._scene).toBe(oldFullLayout.scene._scene);
-            expect(gd._fullLayout._plots).toBe(oldFullLayout._plots);
+            expect(gd._fullLayout._plots.plot).toBe(oldFullLayout._plots.plot);


So it looks like it reconstructs a new _plots object with the same contents vs. just transferring it? Is that what makes the join work correctly?

- to allow calling outside the drawFramework step

rreusser · 2016-10-06T02:10:41Z

None of the changes or comments were blocking, so gets the 💃🏻 from me (once you're happy)!

etpinard · 2016-10-06T02:10:46Z

src/plots/cartesian/index.js

+    return subplotData;
+}
+
+function makeSubplotLayer(plotinfo) {


Important the subplot layer join step is free of references to gd._fullLayout.

This means that other components (e.g. the range slider 😏 ) can call it (with sufficiently mocked plotinfo object with xaxis, yaxis, _id and plotgroup ) and get the subplot layers appended to plotinfo.plotgroup in return !

Hooray for clever code reuse! As long as no too clever. 🎉

rreusser · 2016-10-06T02:51:58Z

Merge looks great, but I did have a question about this:

how does it affect non-cartesian plots? Or does it affect non-cartesian plots? Specifically, does this add or modify the considerations involved in adding object constancy to pie or bar charts? (though bar is cartesian, right? I haven't worked with them much beyond making sure I don't break them)

etpinard · 2016-10-06T03:02:26Z

how does it affect non-cartesian plots?

It should not affect non-cartesian whatsoever. After this PR, all cartesian subplots are inserted in <g cartesianlayer>. You can that pies are inserted here and similarly for other plot types.

All per-subplot routines are now called inside basePlotModules loop e.g. here and here, so I'm pretty confident this PR has cross-subplot type side-effects.

- made useless by #946

- bug noticed when both overlaying x and y axes have `showline: true`. - ... but `Plotly.restyle(gd, 'visible', true, [1])` does not work for the added mock - both bugs have been broken since PR #946 released in 1.18.0

etpinard added 4 commits September 15, 2016 17:12

move subplot creation from makePlotFramework -> Cartesian.plot

c877c27

rm some early-Plotly.plot logic regarding makePlotFramwork

ff26691

plots: add linkSubplot method

40fc7f9

- which handles fullLayout._plots creation and reference updates - rm block in Plots.transition that used to handle that

etpinard added type: maintenance labels Sep 15, 2016

etpinard added 8 commits September 15, 2016 17:55

keep ref to subplot image/shape layers in main drawData

e85a6c7

- so that these ref on fullLayout exist even for non-cartesian trace types.

lint

43d227a

plots: add a cartesian subplot purge step

78faa3f

test: adapt relinkPrivateKeys test

f8017f2

test: adapt legend scroll test

c239602

- this suite used to rely on className 'bg' to grab the legend background element, but now className 'bg' is also used in <g subplot>.

test: adapt gd._ keys ordering

f4aee1e

- doCalcdata is now called before drawFramwework, so the ordering for which _ keys are added to the gd differs slightly.

test: check clip path & dragger counts in cartesian plots

ab96422

etpinard commented Sep 16, 2016

View reviewed changes

etpinard added status: reviewable and removed status: in progress labels Sep 16, 2016

Merge branch 'master' into cartesian-per-subplot

8c09af2

etpinard commented Sep 16, 2016

View reviewed changes

etpinard mentioned this pull request Sep 21, 2016

Range slider second iteration (part 1) #962

Merged

Merge branch 'master' into cartesian-per-subplot

b00d716

replace plotinfo.x() -> plotinfo.xaxis

ae3ab55

etpinard force-pushed the cartesian-per-subplot branch from 2b05978 to ae3ab55 Compare October 4, 2016 22:34

etpinard added this to the v1.18.0 milestone Oct 5, 2016

rreusser approved these changes Oct 5, 2016

View reviewed changes

make makeSubplotLayer agnostic to fullLayout

a708381

- to allow calling outside the drawFramework step

etpinard commented Oct 6, 2016

View reviewed changes

etpinard mentioned this pull request Oct 6, 2016

Bumping d3 #424

Open

rm useless return in makePlotFramework

e78b099

etpinard merged commit 5a08fbc into master Oct 6, 2016

etpinard deleted the cartesian-per-subplot branch October 6, 2016 02:50

etpinard mentioned this pull request Oct 6, 2016

Range slider second iteration (part 2: on-par range plots !!!) #1017

Merged

This was referenced Oct 17, 2016

Cartesian subplot fixes #1049

Merged

Fix dendrogram #1063

Merged

etpinard added a commit that referenced this pull request Jan 10, 2017

rm (now useless) logic for ternary make plot framework

92119bb

- made useless by #946

etpinard mentioned this pull request Jul 6, 2017

Fix overlaying axis layers #1855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cartesian subplot updates using data joins #946

Cartesian subplot updates using data joins #946

etpinard commented Sep 15, 2016

etpinard commented Sep 15, 2016

etpinard Sep 16, 2016

etpinard commented Sep 16, 2016

etpinard Sep 16, 2016

rreusser Oct 5, 2016

etpinard Oct 6, 2016

etpinard Sep 16, 2016

etpinard Sep 16, 2016

etpinard Sep 16, 2016

etpinard Sep 16, 2016

rreusser Oct 5, 2016 •

edited

Loading

etpinard Oct 6, 2016

etpinard Sep 16, 2016

etpinard Sep 16, 2016

etpinard Oct 4, 2016

etpinard Sep 16, 2016

etpinard Oct 4, 2016

rreusser left a comment

rreusser Oct 5, 2016

rreusser Oct 5, 2016

etpinard Oct 6, 2016 •

edited

Loading

rreusser Oct 5, 2016

etpinard Oct 6, 2016

rreusser Oct 5, 2016

rreusser Oct 5, 2016 •

edited

Loading

rreusser Oct 5, 2016

rreusser commented Oct 6, 2016 •

edited

Loading

etpinard Oct 6, 2016 •

edited

Loading

rreusser Oct 6, 2016

rreusser commented Oct 6, 2016 •

edited

Loading

etpinard commented Oct 6, 2016

Cartesian subplot updates using data joins #946

Cartesian subplot updates using data joins #946

Conversation

etpinard commented Sep 15, 2016

etpinard commented Sep 15, 2016

Choose a reason for hiding this comment

etpinard commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Oct 6, 2016 • edited Loading

etpinard Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Oct 6, 2016 • edited Loading

etpinard commented Oct 6, 2016

rreusser Oct 5, 2016 •

edited

Loading

etpinard Oct 6, 2016 •

edited

Loading

rreusser Oct 5, 2016 •

edited

Loading

rreusser commented Oct 6, 2016 •

edited

Loading

etpinard Oct 6, 2016 •

edited

Loading

rreusser commented Oct 6, 2016 •

edited

Loading